Port ScalarFunctionExpr proto via a typed, erasure-free UDF bridge (alt to #23421)#23449
Closed
adriangb wants to merge 2 commits into
Closed
Port ScalarFunctionExpr proto via a typed, erasure-free UDF bridge (alt to #23421)#23449adriangb wants to merge 2 commits into
ScalarFunctionExpr proto via a typed, erasure-free UDF bridge (alt to #23421)#23449adriangb wants to merge 2 commits into
Conversation
`ScalarFunctionExpr` is the one built-in expression whose ser/de needs session-level objects: the embedded `ScalarUDF` round-trips through the `datafusion-proto` extension codec + function registry, and reconstruction needs the session `ConfigOptions`. Those types live *above* `physical-expr-common` in the crate graph, so they cannot appear on the crate-wide `PhysicalExpr::try_to_proto` encode/decode contexts without a dependency cycle. Rather than erase the function type to `dyn Any` on the crate-wide context, the expression declares its own ser/de against two fully-typed bridge traits (`ScalarUdfProtoEncoder` / `ScalarUdfProtoDecoder`) implemented in `datafusion-proto`, where `ScalarUDF` and the codec are both nameable. No `dyn Any` is involved anywhere. Adds the traits, the inherent `try_to_proto` / `try_from_proto` on `ScalarFunctionExpr`, and 6 direct tests over mock bridges. Wiring in `datafusion-proto` follows in the next commit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018LmymuRdTRmGthjMhtCijn
Implement `ScalarUdfProtoEncoder` on `ConverterEncoder` and add a
`ScalarUdfConverterDecoder` implementing `ScalarUdfProtoDecoder` (reproducing
the exact registry-then-codec UDF lookup order). The encode downcast arm and
the decode `ExprType::ScalarUdf` arm now delegate to
`ScalarFunctionExpr::{try_to_proto, try_from_proto}` — the marshalling logic
lives with the expression, with no type erasure.
Wire format is byte-for-byte unchanged; `proto_integration` (190 tests) still
passes, exercising the scalar-UDF round-trip through the new bridge.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018LmymuRdTRmGthjMhtCijn
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
PhysicalExprimplementations to usetry_from_proto/try_to_proto#22418; addresses the same goal as PortScalarFunctionExprto usetry_to_proto/try_from_proto#22430.Alternative to #23421 — same objective (
ScalarFunctionExprdeclares its ownproto ser/de), different resolution of the
ScalarUDFcrate-graph problem. Openingthis so the two shapes can be compared side by side.
Rationale for this change
ScalarFunctionExpris the last expression on the central proto downcast chainwhose serialization needs session-level objects: the embedded
ScalarUDFmust gothrough
PhysicalExtensionCodecon encode and resolve via codec + registry ondecode, and reconstruction needs the session
ConfigOptions.The tension: the crate-wide
try_to_proto/try_from_protocontexts live inphysical-expr-common, which sits belowdatafusion-exprin the crate graph(
datafusion-exprdepends on it forArc<dyn PhysicalExpr>), so those contextscannot name
ScalarUDFwithout a dependency cycle.#23421's answer: absorb
ScalarFunctionExprinto the crate-wide hook by addingdyn Any-erased function-codec channels to the contexts (encode_function<F: Any>/decode_function<F: Any>), with the erasure confined internally and the concretetype verified at the boundary. Headline win: zero special cases in
to_proto.rs.This PR's answer (option B): keep the erasure out of the codebase entirely. The
expression still declares its own ser/de, but against two fully-typed bridge
traits defined next to it in
datafusion-physical-expr(which can nameScalarUDF):datafusion-protoimplements them (it can name every type), and the encode/decodesites downcast to
ScalarFunctionExprand delegate to its inherenttry_to_proto/try_from_proto. Nodyn Anyanywhere.The trade
The two designs sit on opposite sides of one unavoidable fork (dispatching a
session-dependent expression from a bottom-crate trait forces erasure; keeping the
type means keeping a concrete arm):
to_proto.rsspecial casesdyn Anyin the codebaseTypeIdroutingconfig_options()Result, erroring defaultAggregateUDF/WindowUDFThe motivating case for the whole mechanism (#21835,
DynamicFilterPhysicalExprreaching private state) is session-free and rides the crate-wide hook happily
under either design;
ScalarFunctionExpris the only expression dragging sessionobjects across the boundary. B treats it as the one justified exception rather than
reshaping the shared contract to absorb it.
What changes are included in this PR?
Commit 1 —
datafusion/physical-expr:ScalarUdfProtoEncoder/ScalarUdfProtoDecoderbridge traits + module docsexplaining the crate-graph constraint.
ScalarFunctionExpr::try_to_proto/try_from_proto.return_type, child encode/decode error propagation, decode round-trip).Commit 2 —
datafusion/proto:impl ScalarUdfProtoEncoder for ConverterEncoder; the encode downcast arm delegates.ScalarUdfConverterDecoder(same registry-then-codec lookup order); theExprType::ScalarUdfarm delegates.Are these changes tested?
scalar_function.rs::proto_tests.cargo test -p datafusion-proto --test proto_integration: 190 passed. The scalar-UDFround-trip can now only pass through the new bridge.
cargo fmt --allandcargo clippy -p datafusion-physical-expr -p datafusion-proto --all-features -- -D warningsare clean.Are there any user-facing changes?
Wire format is byte-for-byte unchanged. New public API: the two bridge traits and the
two inherent methods (all feature
proto, additive/non-breaking).🤖 Generated with Claude Code
https://claude.ai/code/session_018LmymuRdTRmGthjMhtCijn